Skip to content

fresh binding should shadow the def in expand #143141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 28, 2025

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2025
fn f() -> i8 { 42 }
let f = || -> i16 { 42 };

let a: i16 = m!();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking changes, should we run crate tests?

macro_rules! m {() => ( f() )}
use m;
let b: i16 = m!();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test all permutations of

fn f
let f
macro_rules! m
use m

in all possible orders.
There should be 12 of them (the ones with use m going before macro_rules! m are impossible).

The testing statements let a/b/c/... can then be inserted between all of them.

let a
fn f
let b
let f
let c
macro_rules! m
let d
use m
let e

Then we'll get exhaustive testing.

Upd: i8 -> FnF, i16 -> LetF.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more test cases:

{
  m!(); // in a block before the import
}

macro_rules! m { ... }
use m;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note, we should add additional test cases like without_decl_f and without_closure_f for better coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note on cases in f0~f23:​​

  • For let a: i8 = m!() in f8~f23, the macro expansion's f refers to function f. There are two distinct scenarios:
    1. Using the declared function​ when found at macro definition site:
     fn f20() {
         macro_rules! m {() => ( f() )} // Only resolves to `fn f` at this position
         use m;
         fn f() -> i8 { 42 }
         let f = || -> i16 { 42 };
         let a: i8 = m!();
    }
    This behaves similarly to:
    fn f() {
        let x = 0;
        macro_rules! foo { () => { assert_eq!(x, 0); } }
        let x = 1;
        foo!();
    }
    1. Using the declared function​ when the closure is unavailable:
    fn b12() {
        let a: i8 = m!();
        fn f() -> i8 { 42 }
        let f = || -> i16 { 42 };
        macro_rules! m {() => ( f() )} // Although both exist, `let a` can only use `fn f`
        use m;
    }
  • In other cases, it refers to the closure f.

@petrochenkov
Copy link
Contributor

I don't understand why this works and how it fixes the issue.
The LookAheadMacroDefinition are inserted for all macro definitions, regardless of whether it's macro or macro_rules, and nothing is done for use items.
The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2025
@petrochenkov
Copy link
Contributor

Also "shallow" -> "shadow" in the PR/commit messages and file names.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 1, 2025

The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

There may be a bug if LookAheadMacroDefinition fails to apply correctly.

I don't understand why this works and how it fixes the issue.

LookAheadMacroDefinition stores the macro expansion result and serves as a fallback in resolve_ident_in_lexical_scope, ensuring it takes the correct resolution path:

// The ident resolves to a type parameter or local variable.
return Some(LexicalScopeBinding::Res(self.validate_res_from_ribs(
i,
rib_ident,
*res,
finalize.map(|finalize| finalize.path_span),
*original_rib_ident_def,
ribs,
)));

@bvanjoi bvanjoi changed the title fresh binding should shallow the def after expand fresh binding should shadow the def in expand Jul 1, 2025
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 1, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hygiene of used macro item is weird.
3 participants